fix(core): ignore hidden files when loading skill resources#1002
fix(core): ignore hidden files when loading skill resources#1002jujn wants to merge 5 commits intoagentscope-ai:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #1000 by updating skill resource loading to ignore hidden/dot-prefixed files, preventing unwanted artifacts (e.g., .DS_Store) from being included in AgentSkill resources.
Changes:
- Add filtering in
SkillFileSystemHelper.loadResources(...)to skip dot-prefixed and hidden/unreadable files. - Add a unit test to verify dot-prefixed files are excluded from loaded resources.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| agentscope-core/src/main/java/io/agentscope/core/skill/util/SkillFileSystemHelper.java | Adds hidden/dotfile filtering during resource discovery when loading a skill. |
| agentscope-core/src/test/java/io/agentscope/core/skill/util/SkillFileSystemHelperTest.java | Adds coverage for filtering dot-prefixed files out of loaded resources. |
agentscope-core/src/main/java/io/agentscope/core/skill/util/SkillFileSystemHelper.java
Outdated
Show resolved
Hide resolved
agentscope-core/src/main/java/io/agentscope/core/skill/util/SkillFileSystemHelper.java
Outdated
Show resolved
Hide resolved
agentscope-core/src/test/java/io/agentscope/core/skill/util/SkillFileSystemHelperTest.java
Outdated
Show resolved
Hide resolved
agentscope-core/src/test/java/io/agentscope/core/skill/util/SkillFileSystemHelperTest.java
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@fang-tech PTAL |
| @DisplayName( | ||
| "Should fully cover resource filtering logic (unreadable, dot-files, OS hidden, and" | ||
| + " IOException)") | ||
| void testLoadResources_FiltersAllEdgeCases() throws IOException { |
There was a problem hiding this comment.
Split your tests into individual unit tests for each case.
There was a problem hiding this comment.
Thanks for the suggestion! Considering that the business logic of resource filtering in this section is actually very lightweight. I understand that splitting test cases can make responsibilities more singular, but it can make the test classes cumbersome. Hope you can reconsider?
There was a problem hiding this comment.
If the code of the filter needs to be modified in the future, things will become complicated. We won't be able to precisely determine which aspects of the tests were made unavailable due to our modifications. You might think it's too cumbersome and could appropriately combine them. However, if all the tests are placed in one method, it will impose a burden on future modifications. This is actually a formatting task, and you can use AI to complete it quickly.
Description
Close #1000
Checklist
Please check the following items before code is ready to be reviewed.
mvn spotless:applymvn test)